XWIKI-20953: Edit autosave frequency visibility #4682
Conversation
* WIP, somehow the js fails...
* Updated CSS * Fixed the error in the .js * Moved a flamingo patch to the main CSS of this element (since flamingo is supposed to be the main now anyways).
* Moved a flamingo patch to the main CSS of this element (since flamingo is supposed to be the main now anyways).
michitux
left a comment
michitux
left a comment
There was a problem hiding this comment.
Thank you very much, the UI changes look good to me. I don't agree with the breaking API change, though. Even if it shouldn't be used anywhere (haven't checked), it's a documented public API that we can't break without vote. As the API change isn't related to the UI changes, I would suggest not performing it. I've also searched a bit and found this discussion which suggests that it is okay to say "save at a frequency of once every X minutes" so I'm not sure that "frequency" is that wrong.
| core.edit.wikiToolbar.velocitytext=#* Your velocity code here *# | ||
| core.edit.autosave=Autosave | ||
| core.edit.autosave.every=every | ||
| core.edit.autosave.interval.label=Autosave interval (in mins) |
There was a problem hiding this comment.
The term "mins" is colloquial English, I think we should use a proper formal term like "minutes".
| core.register.successful={0} ({1}): Registration successful. | ||
|
|
||
| ####################################### | ||
| ## until 17.9.0RC1 |
There was a problem hiding this comment.
| ## until 17.9.0RC1 | |
| ## until 17.10.0RC1 |
| enabled: false, | ||
| /** If enabled, how frequent are the savings */ | ||
| frequency: 5, // minutes | ||
| interval: 5, // minutes |
There was a problem hiding this comment.
This is a breaking API change as this is a public API, e.g., documented at https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/FrontendResources/Autosave/. I would suggest not modifying the API and just modifying the displayed text.
| this.setTimeUnit(); | ||
| frequencyLabel.appendChild(document.createTextNode(" ")); | ||
| frequencyLabel.appendChild(this.timeUnit); | ||
| autosaveLabel.appendChild(document.createTextNode("$escapetool.javascript($services.localization.render('core.edit.autosave'))")); |
There was a problem hiding this comment.
It would be much simpler to use the append method here (if this is a proper Element and not just something from Prototype, if not it might be a good reason to migrate away from Prototype).
3 participants
Jira URL
https://jira.xwiki.org/browse/XWIKI-20953
Changes
Description
frequencyoccurences tointervalsince a frequency is not measured in minutes... (and the change of translation key made it easy to do right now :) )autosave.jsScreenshots & Video
Here is the new look of the input:
This is surely not a flawless design, but it fixes the main usability issue without moving too far from what it was.
Executed Tests
Manual tests, see screenshots above.
Could not find the autosave frequency input in the expected page object:
xwiki-platform/xwiki-platform-core/xwiki-platform-test/xwiki-platform-test-ui/src/main/java/org/xwiki/test/ui/po/editor/EditPage.java
Lines 58 to 59 in b646b33
Expected merging strategy